fix: .env loading no longer overrides already-set environment variables - #138
fix: .env loading no longer overrides already-set environment variables#138mattpodwysocki wants to merge 3 commits into
Conversation
.env loading previously applied every key onto process.env with override semantics, so a .env file present in the working directory could take precedence over variables already set by the host process (e.g. MAPBOX_API_ENDPOINT, MAPBOX_ACCESS_TOKEN). Extracted the loading logic into src/utils/loadDotEnv.ts and changed it to skip any key that's already set, matching the intent of Node's own process.loadEnvFile(). Already-set keys are now reported in the startup log message and the config.load_env tracing span instead of being silently skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer feedback on this PR (Mofei) pointed out a real gap in the prior fix: skipping .env keys only when already set doesn't help when the host never set MAPBOX_API_ENDPOINT in the first place -- the common case, since most operators only set MAPBOX_ACCESS_TOKEN and rely on the built-in https://api.mapbox.com/ default. In that case a malicious .env could still set MAPBOX_API_ENDPOINT, since "already set" was false, and the real host-injected access token would still be sent to that endpoint. loadDotEnv now takes a set of protectedKeys that .env may never set at all, regardless of whether the target env already has a value for them. index.ts passes MAPBOX_ACCESS_TOKEN/MAPBOX_API_ENDPOINT. Confirmed live both ways: before this change, a tool call with a malicious .env and no host-set MAPBOX_API_ENDPOINT reached the attacker-controlled endpoint; after it, the same call reaches the real API. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # src/index.ts
|
Update: pushed a fix for the gap Mofei flagged (correctly) in Slack. The original fix only skipped Fixed by making Confirmed live both ways: with the prior fix, a tool call with the malicious New regression tests cover this exact scenario in |
Summary
.envloading previously applied every key ontoprocess.envwith override semantics, so a.envfile present in the working directory could take precedence over variables already set by the host process (e.g.MAPBOX_API_ENDPOINT,MAPBOX_ACCESS_TOKEN).src/utils/loadDotEnv.tsand changed it to skip any key that's already set, matching the intent of Node's ownprocess.loadEnvFile(). Already-set keys are now reported in the startup log message and theconfig.load_envtracing span instead of being silently skipped.mapbox/mcp-server.Test plan
npx vitest run— all tests pass, including new regression tests intest/utils/loadDotEnv.test.tscovering override precedencenpm run buildsucceeds.envin the working directory pointingMAPBOX_API_ENDPOINTelsewhere, and the host already having set both variables, a real tool call (list_styles_tool) still reached the host-configuredapi.mapbox.com, not the.env-supplied value🤖 Generated with Claude Code